Use provider C# reference map with pre-write internalization#10976
Use provider C# reference map with pre-write internalization#10976live1206 wants to merge 9 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
You can try these changes here
|
|
Correction to my follow-up review (point #1). @live1206 rightly pointed out that The real reason the textual So the same marker mechanism you introduced for --generated by Copilot |
Design suggestion: derive body references from expression trees rather than declared deps + source re-parsingFollowing up on the body-reference discussion — the current approach builds graph edges from provider metadata (signatures, properties, fields, attributes, etc.) and then compensates for body-level references three ways: manually-declared The concern is soundness: a pure-metadata graph is an under-approximation of reachability, because a C# type reference can appear anywhere a method body can ( Suggestion: walk the body expression trees instead. The bodies already exist as structured
The fact that the PR re-parses generated source for the client/serialization candidates (rather than walking expression trees) suggests the tree walk wasn't feasible for all bodies — presumably where bodies are raw Not a merge blocker, but I think it's the change that would turn this from "correct-by-testing" into "correct-by-construction." Would also be worth attributing how much of the ~21% comes from skipping body analysis vs. from pre-write internalization (skipping the Roslyn --generated by Copilot |
JoshLove-msft
left a comment
There was a problem hiding this comment.
Found several correctness gaps in the provider-based reference map that can cause generated compilation failures, API compatibility regressions, or missing MRW registrations.
Replace Roslyn-based generated-source reference analysis with provider metadata while preserving generated contracts and customization dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
30cc73c to
980c6ad
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
|
latest regen: Azure/azure-sdk-for-net#60965 (comment) |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
JoshLove-msft
left a comment
There was a problem hiding this comment.
Final pass found one remaining correctness issue in the shared ClientOptions accessibility fix.
--generated by Copilot
|
Reran the PR's full-generation benchmark three times at the documented benchmark revision Environment: Windows 11, .NET 10.0.10, 13th Gen Intel Core i7-13700T. The benchmark fixture lookup required an uncommitted Windows-only path-separator normalization; the measured workload and off/on implementations were unchanged.
Run 2's off measurement is visibly noisy, but every pass confirms a substantial time improvement and the allocation reduction is stable at 54-57%. --generated by Copilot |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
|
|
||
| protected override IReadOnlyList<MethodBodyStatement> BuildAttributesForWrite() | ||
| { | ||
| var visitorAttributes = base.BuildAttributesForWrite().Where(static attribute => !IsBuildableAttribute(attribute)); |
There was a problem hiding this comment.
why is this a special case? Technically, any of the the type providers can be visited so I'm curious why we need to have this special handling here.
| var isResolvable = IsResolvableBuildableType(provider.Type); | ||
| var implementsMrw = ImplementsModelReaderWriter(provider); | ||
| var hasWritableSerialization = HasWritableModelReaderWriterSerialization(provider); | ||
| return isResolvable && implementsMrw && hasWritableSerialization; |
There was a problem hiding this comment.
nit: we can improve the perf of this by exiting early on any one of the conditions
| public override MethodBodyStatement AddCollectionHeaders(ValueExpression prefix, ValueExpression headers) | ||
| => Original.Property(nameof(PipelineRequest.Headers)).Invoke(nameof(PipelineRequestHeaders.Add), [prefix, headers]).Terminate(); | ||
| => Original.Property(nameof(PipelineRequest.Headers)) | ||
| .Invoke(nameof(PipelineRequestHeaders.Add), [prefix, headers], typeArguments: null, callAsAsync: false, extensionType: new PipelineRequestHeadersExtensionsDefinition().Type) |
There was a problem hiding this comment.
do we need to use the cached instance of extensionType ?
| ValueExpression[] parameters = format != null ? [Literal(name), value, delimiter, format] : [Literal(name), value, delimiter]; | ||
| return pipelineRequest.Property(nameof(PipelineRequest.Headers)).Invoke("SetDelimited", parameters).Terminate(); | ||
| return pipelineRequest.Property(nameof(PipelineRequest.Headers)) | ||
| .Invoke("SetDelimited", parameters, typeArguments: null, callAsAsync: false, extensionType: new PipelineRequestHeadersExtensionsDefinition().Type) |
There was a problem hiding this comment.
Same question here
| AddMetadataReference(MetadataReference.CreateFromFile(typeof(ClientResult).Assembly.Location)); | ||
| AddMetadataReference(MetadataReference.CreateFromFile(typeof(BinaryData).Assembly.Location)); | ||
| AddMetadataReference(MetadataReference.CreateFromFile(typeof(JsonSerializer).Assembly.Location)); | ||
| AddCustomCodeMethodDependency("SetDelimited", PipelineRequestHeadersExtensionsDefinition); |
There was a problem hiding this comment.
nit: can we make this an internal static property in PipelineRequestHeadersExtensionsDefinition to avoid having to change this in two places
| generatedTypeNamespace ?? CustomCodeView?.Type.Namespace ?? BuildNamespace(), | ||
| generatedTypeName ?? CustomCodeView?.Name ?? BuildName(), | ||
| DeclaringTypeProvider?.Type.Name); | ||
| GetDeclaringTypeName(DeclaringTypeProvider)); |
There was a problem hiding this comment.
we are doing this calculation in more than one place. Should we consider caching the value ?
| } | ||
| } | ||
|
|
||
| private static bool ShouldKeepGeneratedFile(FileInfo file, HashSet<string> fileNamesToKeep) |
There was a problem hiding this comment.
nit: I think we can simply do away with this helper since it's just delegating to the .Contains method
Summary
Replaces the C# generator's Roslyn-generated reference-map/internalization path with a provider-based reference map that runs before files are written. The generator now computes reachability and accessibility from the in-memory provider model, filters unreachable providers up front, and removes stale generated files by cleaning
src/Generatedbefore writing.What changed
ModelFactoryusings.Performance
Latest full-generation benchmark on the updated benchmark branch (
mtg-manual-name-reduction-latest-bench, commit4cf86686e) averaged 544.5 ms / 53.85 MB with the provider reference map enabled vs 847.4 ms / 138.87 MB with it disabled: 35.7% faster and 61.2% less allocation.Benchmark artifacts/logs were saved locally under
/tmp/typespec-benchmark-results-latest-runs-20260710030912.Azure SDK management-plane local project-reference regen speedups:
Latest measured PR head:
434e76802(test(csharp): rename body dependency reference map tests). Each package was regenerated one-by-one withRegenSdkLocal.ps1 -Parallel 1using the local Azure SDK for .NET project-reference setup.Validation
Azure.ResourceManager.Network,Azure.ResourceManager.DataFactory, andAzure.ResourceManager.AppServicepassed one-by-one with no public API listing diffs.Azure.Data.AppConfiguration(ResponseErrorMRW context coverage)Azure.AI.Projects(custom-code parent-namespace body dependency root coverage)Azure.AI.Extensions.OpenAI(polymorphic/unknown model retention coverage)mainhas been merged into the PR branch.